Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use public conversion methods to avoid reflection #19047

Merged
merged 2 commits into from
Dec 29, 2015
Merged

Use public conversion methods to avoid reflection #19047

merged 2 commits into from
Dec 29, 2015

Conversation

smarterclayton
Copy link
Contributor

Replace many of the remaining s.Convert() invocations with direct
execution, and make generated methods public. Removes 10% of the
allocations during decode of a pod and ~20-40% of the total CPU time.

@k8s-bot
Copy link

k8s-bot commented Dec 23, 2015

GCE e2e build/test failed for commit 8f99260d60ed555d35a172b3f35cd6a12e407155.

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 23, 2015
@k8s-github-robot k8s-github-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 23, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XL

@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 23, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 23, 2015

GCE e2e build/test failed for commit cff20bcb93f24c4927824463377d2b942d949e08.

@k8s-bot
Copy link

k8s-bot commented Dec 23, 2015

GCE e2e test build/test passed for commit 11593d3ea3c9efed0ac301929934b212775a35c8.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 23, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 23, 2015

GCE e2e test build/test passed for commit d2771d74599ba22b2fe3680eff160137d8d76c62.

@smarterclayton smarterclayton added the sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. label Dec 24, 2015
@smarterclayton
Copy link
Contributor Author

Pretty big wins in conversion

benchmark                                      old ns/op     new ns/op     delta
BenchmarkPodConversion-8                       29842         13867         -53.53%
BenchmarkNodeConversion-8                      34235         11467         -66.51%
BenchmarkReplicationControllerConversion-8     25301         13996         -44.68%
BenchmarkPodCopy-8                             4242          4225          -0.40%
BenchmarkNodeCopy-8                            4836          4811          -0.52%
BenchmarkReplicationControllerCopy-8           3968          3884          -2.12%
BenchmarkEncodeCodec-8                         28026         28264         +0.85%
BenchmarkDecodeCodec-8                         85519         73098         -14.52%
BenchmarkDecodeIntoCodec-8                     213909        201374        -5.86%

benchmark                                      old allocs     new allocs     delta
BenchmarkPodConversion-8                       161            81             -49.69%
BenchmarkNodeConversion-8                      196            74             -62.24%
BenchmarkReplicationControllerConversion-8     139            79             -43.17%
BenchmarkPodCopy-8                             25             25             +0.00%
BenchmarkNodeCopy-8                            24             24             +0.00%
BenchmarkReplicationControllerCopy-8           22             22             +0.00%
BenchmarkEncodeCodec-8                         56             56             +0.00%
BenchmarkDecodeCodec-8                         347            299            -13.83%
BenchmarkDecodeIntoCodec-8                     1039           1026           -1.25%

benchmark                                      old bytes     new bytes     delta
BenchmarkPodConversion-8                       12757         9011          -29.36%
BenchmarkNodeConversion-8                      13125         7267          -44.63%
BenchmarkReplicationControllerConversion-8     11829         9235          -21.93%
BenchmarkPodCopy-8                             3280          3280          +0.00%
BenchmarkNodeCopy-8                            3056          3056          +0.00%
BenchmarkReplicationControllerCopy-8           2960          2960          +0.00%
BenchmarkEncodeCodec-8                         5224          5223          -0.02%
BenchmarkDecodeCodec-8                         17679         15374         -13.04%
BenchmarkDecodeIntoCodec-8                     32991         32783         -0.63%

@k8s-bot
Copy link

k8s-bot commented Dec 24, 2015

GCE e2e test build/test passed for commit 6d5d7f9597532b5a11174867f859fcee5fa0cb6f.

@smarterclayton
Copy link
Contributor Author

@k8s-bot unit test this

@smarterclayton smarterclayton mentioned this pull request Dec 24, 2015
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 24, 2015
wojtek-t
wojtek-t reviewed Dec 28, 2015
View reviewed changes
pkg/runtime/conversion_generator.go
@@ -529,6 +588,14 @@ func (g *conversionGenerator) writeFooter(b *buffer, indent int) {
b.addLine("}\n", indent)
}

func (g *conversionGenerator) conversionFunctionCall(inType, outType reflect.Type, scopeName string, args ...string) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach - thanks!

Since this is replacing usage of "existsDedicatedConversionFunction" - can you please replace the only remaining call to it and remove it?

@wojtek-t
Copy link
Member

It generally LGTM. I added one comment. Feel free to self-apply lgtm label after applying comment and rebasing. Thanks!

BTW - I will be leaving for paternity leave in O(days) for about a month, so I can't guarantee any responsiveness around that time...

@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 29, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 29, 2015
@wojtek-t
Copy link
Member

@smarterclayton - there is something wrong with the PR (I can't see the diff - there were also some other PRs like that, so it's not your fault). I guess it was just a rebase, so please reapply the lgtm label (I can't see the diff)

@smarterclayton
Copy link
Contributor Author

It was just a trivial rebase - new conversion method got generated and conflicted because of the existing generation.

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 29, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e test build/test passed for commit 4d672c3.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e test build/test passed for commit 4d672c3.

@smarterclayton
Copy link
Contributor Author

@k8s-bot unit test this

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e build/test failed for commit 4d672c3.

@smarterclayton
Copy link
Contributor Author

ok to test

@smarterclayton smarterclayton mentioned this pull request Dec 29, 2015
Closed
@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e build/test failed for commit 4d672c3.

@smarterclayton
Copy link
Contributor Author

@k8s-bot unit test this

@smarterclayton
Copy link
Contributor Author

ok to test

@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e test build/test passed for commit 4d672c3.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e test build/test passed for commit 4d672c3.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e test build/test passed for commit 4d672c3.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 29, 2015
Merge pull request #19047 from smarterclayton/avoid_reflection_during…
cf35905 
…_convert

Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit cf35905 into kubernetes:master Dec 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wojtek-t wojtek-t

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@smarterclayton @k8s-bot @k8s-github-robot @wojtek-t @k8s-teamcity-mesosphere @googlebot @brendandburns